Update OZ solhint custom rules #5794
Conversation
|
|
For reference, there is already this PR that deals with linter rules: #5497 |
There was a problem hiding this comment.
Ideally I would like to add another rule to verify that Interfaces can only import other interfaces
AFAIK, this is already something solidity enforces. We don't need the linter to check taht if the compiler rejects it anyway.
Edit: that is true of "inheritance", importing is a different thing.
Amxx
force-pushed
the
update-solhint-custom-rules
branch
from
d1b7fad to
ca766d7
Compare
Amxx
added
automation
Amxx
changed the title
| @@ -23,7 +23,7 @@ abstract contract Multicall is Context { | |||
| * @dev Receives and executes a batch of function calls on this contract. | |||
| * @custom:oz-upgrades-unsafe-allow-reachable delegatecall | |||
| */ | |||
| function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) { | |||
| function multicall(bytes[] calldata data) public virtual returns (bytes[] memory results) { | |||
There was a problem hiding this comment.
If I remember correctly, this is purposely external since it doesn't have an internal use. Users could just call the functions they want directly
There was a problem hiding this comment.
By default, all public facing functions should be public so that overrides can call super.
If we have exceptions, the questions we need to answer are:
- is there any reason we think overrides won't need to call super?
- is there any risks in making that internally callable?
I'm not sure what override we would want to do, but I'd say calling super would be reasonnable here ... so really the question that remains is 2.
If function A internally calls multicall(...) (instead of doing this.multicall(...)) that would allow arbitrary function execution with the same caller as A's initial context. That is also what we would achieve by directly calling the corresponding function (assuming they are public and not external).
But yes, let review all the external->public changes, and we can disable the rule selectivelly if we have a good reason to keep the functions externals
interface-namescustom rule which is now covered by solhint's already includedinterface-starts-with-iinterface-only-external-functionsrule.Note that 2, 3, and 4 require breaking changes that can be potential candidates for 6.0
Ideally I would like to add another rule to verify that Interfaces can only import other interfaces